-
Notifications
You must be signed in to change notification settings - Fork 20
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add semester_id query to bdb and make student_contact unique for all semesters #3645
Conversation
falbru
commented
Oct 2, 2024
- Add semester_id query to bdb
- make student_contact unique for all semesters
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! Just wondering if the ID will be the year, I think that makes more sense
class StudentCompanyContactSerializer(BasisModelSerializer): | ||
class Meta: | ||
model = StudentCompanyContact | ||
fields = ("id", "company_id", "student_id", "semester_id") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the semester_id supposed to be the year? (In the example it's "1"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's the ID of the Semester object
lego/apps/companies/views.py
Outdated
def _list(self, request, *args, **kwargs): | ||
semester_id = request.query_params.get('semester_id', None) | ||
|
||
serializer = self.get_serializer(self.queryset, many=True, context={'semester_id': semester_id}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there some way to just update the context (so we don't lose request and the other couple things passed down by default)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point! I'll look into it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I agree with Jonas, it would be neat using the year instead something like bdb?semester=spring-2024,
but if we get a decent way to switch between semesters in frontend it also does not really matter.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3645 +/- ##
==========================================
+ Coverage 89.14% 89.16% +0.01%
==========================================
Files 684 685 +1
Lines 21537 21574 +37
==========================================
+ Hits 19200 19237 +37
Misses 2337 2337 ☔ View full report in Codecov by Sentry. |
I'll change the query paramter later. Kind of bored of this PR rn and want it merged |